Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make a monomorphic and monofunctorial distage-example variants #447

Merged
merged 12 commits into from
Jun 19, 2024

Conversation

Ivoyaa
Copy link
Contributor

@Ivoyaa Ivoyaa commented Apr 13, 2024

An attempt to implement 7mind/izumi#1525

@Ivoyaa Ivoyaa marked this pull request as ready for review April 13, 2024 14:37
@Ivoyaa Ivoyaa requested a review from neko-kai as a code owner April 13, 2024 14:37
@pshirshov
Copy link
Member

pshirshov commented Apr 13, 2024

There are two issues with this P/R:

  1. This code is still polymorphic. It's polymorphic code abstracting from monofunctor it runs on.
  2. This P/R just replaces existing code while we want to have a separate example

I would suggest the following:

  1. Let's move our existing code into a directory like distage-example-bifunctor-tf
  2. Let's put your code into new directory, distage-example-monofunctor-tf
  3. We still need distage-example-cats-monomorphic which will use the effect (whichever it is, let's assume cats-effect) directly.

@Ivoyaa
Copy link
Contributor Author

Ivoyaa commented Apr 15, 2024

There are two issues with this P/R:

  1. This code is still polymorphic. It's polymorphic code abstracting from monofunctor it runs on.
  2. This P/R just replaces existing code while we want to have a separate example

I would suggest the following:

  1. Let's move our existing code into a directory like distage-example-bifunctor-tf
  2. Let's put your code into new directory, distage-example-monofunctor-tf
  3. We still need distage-example-cats-monomorphic which will use the effect (whichever it is, let's assume cats-effect) directly.

Yep, I see. About replacing code - I initially thought to do it as Kai suggested in the original issue. To make PR here and then put it another repository, but separate directories sound good as well. To clarify: they also have to be in separate sbt modules?

@pshirshov
Copy link
Member

Yep, I think so.

@Ivoyaa Ivoyaa marked this pull request as draft April 27, 2024 06:55
@Ivoyaa Ivoyaa force-pushed the feature/monomorphic-distage-example branch from a988474 to 0e28225 Compare April 27, 2024 09:19
@Ivoyaa
Copy link
Contributor Author

Ivoyaa commented Apr 27, 2024

Sorry for enormous lines of code changes. They are caused by moving and copying src/main/resources/META-INF.native-image/docker-java.

Also, I had problems with launching GraalVM build locally, so did not actually check that commands from CI work as expected.

@Ivoyaa Ivoyaa changed the title Make a monomorphic distage-example variant Make a monomorphic and monofunctorial distage-example variants Apr 27, 2024
@Ivoyaa Ivoyaa marked this pull request as ready for review April 27, 2024 13:08
Copy link
Member

@neko-kai neko-kai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! 🙏 There are a still a few small issues left before merging, though

@Ivoyaa Ivoyaa marked this pull request as draft May 1, 2024 08:57
@Ivoyaa Ivoyaa marked this pull request as ready for review May 1, 2024 10:13
@Ivoyaa
Copy link
Contributor Author

Ivoyaa commented May 1, 2024

Thank you very much! 🙏 There are a still a few small issues left before merging, though

Thanks for your time! Fixed most of the minor issues.
Still not exactly sure about CI native image step. I divided it into three consequential steps but have few doubts whether it is a correct way to do it. Probably, it is better to launch them in parallel or just check bifunctor variant alone.

@neko-kai
Copy link
Member

neko-kai commented May 1, 2024

Probably, it is better to launch them in parallel or just check bifunctor variant alone.

@Ivoyaa Yeah, check native image step only for bifunctor.

@Ivoyaa
Copy link
Contributor Author

Ivoyaa commented May 6, 2024

@neko-kai @pshirshov hi! Fixed the CI step, can you, please, launch it again?

upd: oh, its automatic

@Ivoyaa
Copy link
Contributor Author

Ivoyaa commented May 13, 2024

@pshirshov @neko-kai hi! Can you review this PR again, please? 👀

@neko-kai neko-kai merged commit e81f5ad into 7mind:develop Jun 19, 2024
2 checks passed
@neko-kai
Copy link
Member

@Ivoyaa Thank you! Sorry for the wait for merging 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants